ref(node): Streamline mongoose instrumentation#21481
Conversation
logaretm
force-pushed
the
awad/js-2389-streamline-opentelemetryinstrumentation-mongoose
branch
from
b56fa12 to
f5972de
Compare
| "typescript/no-explicit-any": "off", | ||
| "no-unsafe-member-access": "off", | ||
| "no-this-alias": "off" |
There was a problem hiding this comment.
These are very common in instrumentations, so I wanted to remove the blanket eslint-disable we have and instead target the problematic rules.
There was a problem hiding this comment.
so far I removed the eslint-disable but also fully removed the exemption here (updating the instrumentations were necessary so it passes linting). on second thought might not be worth doing since we are porting to orchestrion soon, but I am not sure yet how much of the current instrumentation code is gonna stay. wdyt?
There was a problem hiding this comment.
I think it depends, mongoose needs those off so I assume other similar instrumentations will. Once we switch to orchestrion across the board we can re-tighten this.
I realize that by saying that, someone has to remember to do that 😬 but then those rules would be harmless there since they won't be hiding anything.
There was a problem hiding this comment.
yes other instrumentations needed it too (I just updated them so they no longer need the exemptions). my question was more is this worth doing now or do we wait until we are close to the end state (i.e. after orchestrion port)? do you think these will mostly be noops once we move to orchestrion?
There was a problem hiding this comment.
Yea I think they will be noops unless orchestrion cares about types, which i don't think it does.
size-limit report 📦
|
logaretm
requested review from
JPeer264 and
andreiborza
and removed request for
a team
| attributes, | ||
| parentSpan, | ||
| ); | ||
| const span = self._startSpan(this._model.collection, this._model?.modelName, 'aggregate', parentSpan); |
There was a problem hiding this comment.
can we possibly rewrite this to use startSpan(..., () => callback) or a similar thing? Using our active span wrapper has some benefits as that has built in error handling etc 🤔
There was a problem hiding this comment.
I noticed the initial implementation used inactive spans so I didn't want to change semantics here and opted to use the startInactiveSpans to keep things as they are, minimal changes and all of that.
Now, I don't really see a lot of spans getting affected by switching to startSpan. The only thing that would run inside mongoose spans would be a mongodb driver calls, so I believe that is a good thing for them to be parented to it.
I will try switching to a mix of startSpan and startSpanManual (for callback-based paths) and see if that works.
There was a problem hiding this comment.
It changes more than I expected, so we change some error messages (which we can redecorate) but then there is the thenable issue....
Some methods, like 8.21+ document's updateOne/deleteOne don't return a real Promise, they return a lazy Query. The instrumentation has to hand that Query back to the caller un-executed (we just tag it so the eventual .exec() isn't instrumented twice).
This interacts badly with startSpan/startSpanManual as handleCallbackErrors calls .then() on anything thenable, and for a mongoose Query, .then() would execute it. So wrapping these in startSpan runs the query prematurely.
I tested this against mongoose 8.21 and it did produce this issue, so it seems like we can't do that. I will add tests for these cases since our CI wouldn't have caught it.
For parenting I could just do withActiveSpan but that's just a single call change.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
logaretm
force-pushed
the
awad/js-2389-streamline-opentelemetryinstrumentation-mongoose
branch
3 times, most recently
from
b898af0 to
8b25b2d
Compare
Replace the OpenTelemetry tracing APIs in the vendored mongoose instrumentation with Sentry primitives (startInactiveSpan, getActiveSpan, span.setStatus) and bake the span origin in directly. Also drops the config paths that are dead in Sentry's context (dbStatementSerializer, requireParentSpan, suppressInternalInstrumentation) and the SemconvStability dual-emission machinery, keeping only the OLD attribute set. The OpenTelemetry instrumentation base/module-patching layer is kept.
With the OTel responseHook/dbStatementSerializer paths removed, the only config the SDK uses is the base InstrumentationConfig. Collapse MongooseInstrumentationConfig to that and drop the now-unused SerializerPayload, DbStatementSerializer, ResponseInfo and MongooseResponseCustomAttributesFunction types.
Remove ATTR_DB_STATEMENT (fed the removed dbStatementSerializer path) and DB_SYSTEM_NAME_VALUE_MONGODB (only used by the dropped stable-semconv branch). The remaining constants are the OLD attribute set the instrumentation still emits.
Port the upstream OTel mongoose test coverage as unit tests that drive a fake mongoose module through MongooseInstrumentation and assert the produced Sentry spans. Covers save, query exec, aggregate, insertMany, bulkWrite, document update methods (v8.21+), remove (v5/v6), error status, parent-span linking and unpatch.
Type the vendored instrumentation's `module` parameter (MongooseModule / MongooseModuleExports) instead of `any`, so the patch/unpatch logic is checked against the mongoose shape. Adjust lint config accordingly.
…ion suite Exercise more mongoose operations against the real mongodb-memory-server so the live suite asserts span output (name, OLD attributes, op, origin) for aggregate, insertMany and bulkWrite in addition to save/findOne.
Wrap the mongoose operation in `withActiveSpan` so the span is active while the underlying driver call runs. This parents the mongodb driver spans under the corresponding mongoose span (e.g. mongoose.User.save -> mongodb insert) instead of leaving them as siblings. `withActiveSpan` returns the callback result untouched, so it does not call `.then()` on lazy mongoose Query thenables (unlike startSpan/startSpanManual, which would execute them prematurely). The active window is synchronous-only, so unrelated spans are not parented to the mongoose span.
Add integration suites pinning mongoose 7, 8 and 9 (alongside the existing v6 suite) so every supported version branch is exercised against a real mongoose: contextCaptureFunctions7 (v7), the 8.21+ document-method path (v8) and the latest major (v9). The v8 suite guards the lazy-Query trap directly: it builds a document `updateOne` without awaiting it and asserts the document is not modified. The instrumentation must hand the lazy Query back un-executed; running it (as `startSpan`/`startSpanManual` would, by calling `.then()` on the returned thenable) causes a premature write that fails the test.
The instrumentation supports the callback form (mongoose 5/6) by forwarding the original `arguments` and swapping the callback slot by position. Exercise `save(callback)` end-to-end against real mongoose 6 and assert the callback receives the saved document, so the positional handling is covered (the unit test's fake finds the callback by type, not position).
mongoose 9 requires Node >=20.19, so on the Node 18 CI job the v9 scenario
crashed at runtime (save rejected, partial spans) and the suite failed. Wrap
it in `conditionalTest({ min: 20 })` so it's skipped on older Node, matching
mongoose 9's own engine constraint.
logaretm
force-pushed
the
awad/js-2389-streamline-opentelemetryinstrumentation-mongoose
branch
from
c966f01 to
e2899b2
Compare
| "typescript/no-explicit-any": "off", | ||
| "no-unsafe-member-access": "off", | ||
| "no-this-alias": "off" |
There was a problem hiding this comment.
so far I removed the eslint-disable but also fully removed the exemption here (updating the instrumentations were necessary so it passes linting). on second thought might not be worth doing since we are porting to orchestrion soon, but I am not sure yet how much of the current instrumentation code is gonna stay. wdyt?
…n tests The fake-module unit suite mostly duplicated operation/attribute coverage now provided by the real mongoose v6/v7/v8/v9 integration suites, plus some internal-only checks (version gating, unpatch, $save aliasing). Behavior is better verified against real mongoose, so remove the fake suite.
Add a failing (validation) save to the v6 integration scenario and assert it still produces a `mongoose.RequiredDoc.save` span with the expected origin and an error status. This restores the error-path coverage previously held by the removed fake unit suite, now verified against real mongoose.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cbb3a2c. Configure here.
| const lazyDoc = await new BlogPost({ title: 'Original', body: 'b', date: new Date() }).save(); | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| lazyDoc.updateOne({ title: 'PrematurelyExecuted' }); | ||
| await new Promise(resolve => setTimeout(resolve, 250)); |
There was a problem hiding this comment.
Fixed sleep invites flaky regression
Low Severity
The lazy-updateOne guard waits with setTimeout(..., 250) instead of a concrete completion signal. Slow CI or delayed writes can let a premature execution finish after the sleep, so the scenario may pass even when instrumentation incorrectly runs the query early.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit cbb3a2c. Configure here.
- Assert the mongodb driver span is parented under the mongoose span (the withActiveSpan behavior) in the v6 suite, which runs on all supported Node versions. - Exercise document updateOne/deleteOne on mongoose 9 and assert their spans. On v9 these aren't doc-method-patched (needsDocumentMethodPatch only matches 8.x) but are still correctly instrumented via the patched Query.exec path.
| function setErrorStatus(span: Span, error: MongooseError): void { | ||
| span.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| code: SPAN_STATUS_ERROR, | ||
| message: `${error.message} ${error.code ? `\nMongoose Error Code: ${error.code}` : ''}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: The Mongoose instrumentation no longer calls span.recordException() on errors, preventing full exception details like stack traces from being recorded on the Sentry span for failed database operations.
Severity: MEDIUM
Suggested Fix
In packages/node/src/integrations/tracing/mongoose/vendored/utils.ts, reintroduce the span.recordException(err) call within the .catch block of the handlePromiseResponse function, alongside the existing setErrorStatus(span, err) call. This will align its behavior with other database instrumentations and ensure full error details are captured.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/mongoose/vendored/utils.ts#L44-L49
Potential issue: The refactoring of the Mongoose instrumentation to use Sentry's native
span APIs has removed the call to `span.recordException(error)` within the error
handling logic of `handlePromiseResponse`. While the error status is set on the span via
`setErrorStatus`, the full exception object, including the stack trace, is no longer
recorded. This is inconsistent with other database instrumentations like Postgres and
Redis, which continue to use `span.recordException()`. As a result, when any Mongoose
operation fails, valuable debugging information will be missing from the corresponding
Sentry span, hindering error analysis.
3 participants
Streamlines the vendored
mongooseinstrumentation to use Sentry's span APIs instead of the OpenTelemetry tracing API, and removes the code paths that are dead in Sentry's context.I also ported OTel's own integration tests of mongoose and added my own assertions to it.
I didn't fix the older semantic attributes as I believe that would be a breaking change right now.
One semantic change is now mongodb driver spans will be parented to mongoose spans, before they were siblings which i think is not correct trace-wise.
Fixes #20737